-
Notifications
You must be signed in to change notification settings - Fork 55
[ABA impact] - Re-enable Fiat Price Impact + minor optimisations #1986
Conversation
|
Hey @W3stside , is this OK to see the message along with a price impact value? I'm clarifying due to it is a bit unclear to me what else, besides displaying price impacts even without USD estimations, to test. |
hmm interesting, let me review this |
Hey @W3stside , great! Now the message is displayed only if a price impact is missing. blink.mp4 |
@gnosis/gp-qa @gnosis/gp-frontend I am preemptively merging please post merge review |
d6f834e
to
0abf00c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @W3stside , I see an eternal loader when I select From token and its amount, but has not yet selected To token. |
Also, I can see an eternal loader when all tokens and amounts are specified. Sometimes I can't even press on the Swap button when the impact is being loaded (in regular mode): This might be a result of failed BE requests or so.. But as an option, could we set something like a timer for this loading indicator? |
@elena-zh infinite loader is fixed in latest commit |
1. bail out early on do not calc
7575ef4
to
2d3e2b4
Compare
Hey @W3stside , I have been able to face this: I see Price impact in percentage, but get price impact unknown warning. This is due to buy amount is too low to cover fees for the inverse sell order. I think we shouldn't show percentage in this case. The same is when there is insufficient liquidity for an inverse trade Actually, it might be displayed even when there is enough liquidity for both trades, and amounts cover fees |
Also, I noticed that tokens' USD estimation appears disappears on the form (XDAI network) |
Reported as a separate issue #1989 |
Changes LGTM besides this warning flashing issue @W3stside , let me know please if I need to create a separate issue for this case. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merging after successful build (addressed @Anxo comments) |
@anxolin , we have an issue for this on the board: #1979 Actually, the max 49.99% price impact appears when there is insufficient liquidity for the inverse trade |
Re-enables the fiat price impact.
Testing